Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix fatal error on constant('') #3013

Merged
merged 18 commits into from
May 31, 2024
Merged

Fix fatal error on constant('') #3013

merged 18 commits into from
May 31, 2024
+176 −11

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Apr 12, 2024 •
edited

closes phpstan/phpstan#10867

there are only 2 other callers for this very same method in the codebase, and there we already early exit on '':

e.g.

phpstan-src/src/Type/Php/DefinedConstantTypeSpecifyingExtension.php

Lines 50 to 60 in 5c3a711

if (
!$constantName instanceof ConstantStringType
|| $constantName->getValue() === ''
) {
return new SpecifiedTypes([], []);
}
return $this->typeSpecifier->create(
$this->constantHelper->createExprFromConstantName($constantName->getValue()),
new MixedType(),
$context,

@staabm staabm marked this pull request as ready for review April 12, 2024 19:21
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

ondrejmirtes
ondrejmirtes requested changes Apr 12, 2024
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a stub for Name constructor and all Name subclasses constructors that enforces non-empty-string.

@staabm
Copy link
Contributor Author

staabm commented Apr 12, 2024

using non-empty-string names everywhere would spread across the whole codebase. I fixed the places which worked without ugly catches and baselined the remaining ones

@staabm
Copy link
Contributor Author

staabm commented Apr 12, 2024 •
edited

tested locally with regarding __construct() in all known Name classes

➜  phpstan-src git:(bug10867) ✗ cat test4.php 
<?php

$n = new \PhpParser\Node\Name('');

$r = new \PhpParser\Node\Name\Relative('');

$fq = new \PhpParser\Node\Name\FullyQualified('');
➜  phpstan-src git:(bug10867) ✗ php bin/phpstan analyze test4.php --debug
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
/Users/staabm/workspace/phpstan-src/test4.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  Line   test4.php                                                                                                                                                   
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  3      Parameter #1 $name of class PhpParser\Node\Name constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.                 
         💡 Type #1 from the union: '' is empty.                                                                                                                     
  5      Parameter #1 $name of class PhpParser\Node\Name\Relative constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.        
         💡 Type #1 from the union: '' is empty.                                                                                                                     
  7      Parameter #1 $name of class PhpParser\Node\Name\FullyQualified constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.  
         💡 Type #1 from the union: '' is empty.                                                                                                                     
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 


                                                                                                                        
 [ERROR] Found 3 errors

also created a upstream PR for PHPParser: nikic/PHP-Parser#993

@ondrejmirtes
Copy link
Member

using non-empty-string names everywhere would spread across the whole codebase.

Yes, that's the idea. I'm not looking for a quick fix but a correct fix. Please do not baseline anything. Baseline is not supposed to grow.

@staabm staabm mentioned this pull request Apr 12, 2024
Merged
@staabm
Copy link
Contributor Author

staabm commented Apr 13, 2024

looks like this will be a breaking change and can only be merged for 2.0?
should the crash fix be separated?

ondrejmirtes
ondrejmirtes requested changes Apr 14, 2024
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more careful - in some places where we gain type safety thanks to the new PHPDocs, you instantly throw it away by throwing ShouldNotHappenException.

conf/config.neon Outdated Show resolved Hide resolved
src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php Outdated Show resolved Hide resolved
src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php Outdated Show resolved Hide resolved
src/Type/Php/ConstantHelper.php Outdated Show resolved Hide resolved
src/Rules/Generics/TemplateTypeCheck.php Outdated Show resolved Hide resolved
src/Reflection/InitializerExprContext.php Outdated
@@ -127,6 +138,10 @@ public static function fromStubParameter(

public static function fromGlobalConstant(ReflectionConstant $constant): self
{
if ($constant->getNamespaceName() === '') {
throw new ShouldNotHappenException('Namespace cannot be empty.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be tackled by a better typehint in BetterReflection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in Roave/BetterReflection#1428

@ondrejmirtes
Copy link
Member

It'd be great if you could finish this one. Thanks.

@staabm
Copy link
Contributor Author

staabm commented May 31, 2024 •
edited

I am on it right in this moment :)

sorting out dependencies atm

This was referenced May 31, 2024
Merged
Merged
@staabm
Copy link
Contributor Author

staabm commented May 31, 2024

I think this one is ready to land. the last remaining error should be fixed with Roave/BetterReflection#1428

@ondrejmirtes ondrejmirtes merged commit c9faa60 into phpstan:1.11.x May 31, 2024
420 of 445 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the bug10867 branch May 31, 2024 16:42
staabm
staabm commented May 31, 2024
View reviewed changes
build/stubs/PhpParserName.stub

use PhpParser\NodeAbstract;

class Name extends NodeAbstract
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to remove this stub after the next php-parser release because of nikic/PHP-Parser#993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@staabm @phpstan-bot @ondrejmirtes